Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Rewrite the Tensorboards Web App charm following the sidecar pattern #74

Merged
merged 28 commits into from
Jun 28, 2023
Merged

feat: Rewrite the Tensorboards Web App charm following the sidecar pattern #74

merged 28 commits into from
Jun 28, 2023

Conversation

orfeas-k
Copy link
Contributor

@orfeas-k orfeas-k commented Jun 13, 2023 •
edited
Loading

This PR rewrites the Tensorboards Web App (TWA) charm following the sidecar pattern
In short:

  • Update pip packages' versions (bumped SDI version in TWA charm)
  • Update README.md file to match the charm where it is located
  • Introduce CONTRIBUTING.md file
  • Add upstream manifests as Jinja2 templates
  • Set charm's status to blocked when there is no ingress relation
  • Rewrite the charm following the sidecar pattern
  • Adjust joint charms' deploy test to match changes
  • Add integration tests for TWA charm
  • Run TWA integration tests in Github CI
  • Drop TWA container port from config option
  • Use production as default for config's Backend-mode

Upgrade instructions

  1. Trust the charm
juju trust tensorboards-web-app
  1. Refresh the charm providing:
    i. the image since oci-image was changed to tensorboards-web-app-image.
    ii. the path to the local charm or the corresponding channel (for charms released in charmhub)
juju refresh tensorboards-web-app --resource tensorboards-web-app-image=kubeflownotebookswg/tensorboards-web-app:v1.7.0 --path="./tensorboards-web-app_ubuntu-version-arch.charm"

or

juju refresh tensorboards-web-app --resource tensorboards-web-app-image=kubeflownotebookswg/tensorboards-web-app:v1.7.0 --channel=track/risk

@orfeas-k orfeas-k requested a review from a team as a code owner June 13, 2023 15:25
@beliaev-maksim beliaev-maksim marked this pull request as draft June 13, 2023 15:44
@orfeas-k orfeas-k changed the title WIP: feat: Rewrite the Tensorboards Web App charm following the sidecar pattern feat: Rewrite the Tensorboards Web App charm following the sidecar pattern Jun 14, 2023
@orfeas-k orfeas-k marked this pull request as ready for review June 14, 2023 12:15
@orfeas-k orfeas-k closed this Jun 15, 2023
@orfeas-k orfeas-k deleted the orfeas-twa-rewrite-sidecar branch June 15, 2023 08:34
@orfeas-k orfeas-k restored the orfeas-twa-rewrite-sidecar branch June 15, 2023 08:35
@orfeas-k orfeas-k reopened this Jun 15, 2023
DnPlas
DnPlas reviewed Jun 15, 2023
View reviewed changes
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @orfeas-k, some comments.

charms/tensorboards-web-app/CONTRIBUTING.md Show resolved Hide resolved
charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/integration/test_charm.py Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Show resolved Hide resolved
@ca-scribner
Copy link
Contributor

Extremely pedantic suggestion for the upgrade instructions :)

juju refresh tensorboards-web-app ... --channel=version/mode

should be

juju refresh tensorboards-web-app ... --channel=track/risk

Doesn't change the function, but gets the terminology consistent with elsewhere.

ca-scribner
ca-scribner approved these changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some suggested changes, but overall looks really good!

charms/tensorboards-web-app/CONTRIBUTING.md Outdated Show resolved Hide resolved
charms/tensorboards-web-app/requirements-integration.in Outdated Show resolved Hide resolved
charms/tensorboards-web-app/requirements.in Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/templates/auth_manifests.yaml.j2 Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/tests/unit/test_operator.py Show resolved Hide resolved
.github/workflows/integrate.yaml Outdated Show resolved Hide resolved
ca-scribner
ca-scribner suggested changes Jun 15, 2023
View reviewed changes
Copy link
Contributor

@ca-scribner ca-scribner left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoops. misclick on the previous review!

orfeas-k added a commit to canonical/kubeflow-roles-operator that referenced this pull request Jun 19, 2023
@orfeas-k
fix: Remove tensorboards-web-app aggregation roles
f776a12 
Remove tensorboards-web-app aggregation roles since TWA charm now
follows the sidecar pattern with Pebble and can apply aggregation roles
itself. See canonical/kubeflow-tensorboards-operator#74
ca-scribner
ca-scribner previously approved these changes Jun 23, 2023
View reviewed changes
@orfeas-k
fixup! twa: Rewrite charm with sidecar pattern
35a05b0 
- Modify on_remove to use krh.delete() function
- Rename err to error in all instances
- Remove container connection check since it is done by update_layer()
- Set status in _on_event instead of _configure_mesh()
- Stop logging inside check_leader
@orfeas-k
fixup! tests(twa): Rewrite TWA unit tests
d37f702
DnPlas
DnPlas reviewed Jun 26, 2023
View reviewed changes
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments @orfeas-k, after that I think I can approve.

charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Outdated Show resolved Hide resolved
charms/tensorboards-web-app/src/charm.py Show resolved Hide resolved
DnPlas
DnPlas approved these changes Jun 28, 2023
View reviewed changes
Copy link
Contributor

@DnPlas DnPlas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @orfeas-k , great initial contribution!

@orfeas-k orfeas-k merged commit bc445de into main Jun 28, 2023
@orfeas-k orfeas-k deleted the orfeas-twa-rewrite-sidecar branch June 28, 2023 12:58
orfeas-k added a commit to canonical/kubeflow-roles-operator that referenced this pull request Jun 28, 2023
@orfeas-k
fix: Remove tensorboards-web-app aggregation roles (#58)
b034a36 
Remove tensorboards-web-app aggregation roles since TWA charm now
follows the sidecar pattern with Pebble and can apply aggregation roles
itself. See canonical/kubeflow-tensorboards-operator#74
@orfeas-k orfeas-k mentioned this pull request Jan 31, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@i-chvets i-chvets i-chvets left review comments

@phoevos phoevos phoevos left review comments

@ca-scribner ca-scribner ca-scribner left review comments

@kimwnasptd kimwnasptd kimwnasptd left review comments

@DnPlas DnPlas DnPlas approved these changes

Assignees
No one assigned
Labels
Libraries: OK
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@orfeas-k @ca-scribner @kimwnasptd @DnPlas @phoevos @i-chvets